fix(gcp): price tiered SKUs from the first paid tier by IAmJSD · Pull Request #433 · LeanerCloud/cloud-commitments-go

IAmJSD · 2026-10-16T02:26:25Z

Root cause

extractSQLPriceFromSKU (Cloud SQL), extractPriceFromSKU (Memorystore) and extractStoragePriceFromSKU (Cloud Storage) priced a SKU from TieredRates[0] only. A SKU whose first tier is a free allowance ([0, >0]) came back as price 0, and the price == 0 → continue guard in each extract*PricingFromSKUs loop then dropped it, so the region/tier got no price instead of the paid one.

Fix

New skumatch.PaidRate(*cloudbilling.PricingExpression) *cloudbilling.TierRate returns the first tier with a non-zero unit price (skipping nil tiers and tiers with no UnitPrice), or nil when every tier is free.
The three extractors call it in place of TieredRates[0]. Everything after that (nanos math, Cloud Storage unit conversion, currency, slot) is unchanged.
I picked "price from the first non-zero tier" (the first option in the issue's acceptance criteria). The catalog lists tiers by ascending StartUsageAmount, and a workload large enough to need a commitment is past the free allowance, so the first paid tier is the rate it pays at the margin. This is a documented assumption. I did not check the live Billing Catalog for a real Cloud SQL or Memorystore SKU shaped like this, and I made no GCP API calls.
SKUs where every tier is free are still skipped, as before. A negative price is still non-zero, so the existing "negative catalog price" tests behave as before.

Closes #242

Verification

All of this is unit/fixture evidence built from hand-written cloudbilling.Sku values. No real GCP catalog data was used.

Fail-before (new tests run on main's extractors, before the fix):

--- PASS: TestPaidRate
--- FAIL: TestExtractSQLPricing_FreeFirstTierUsesPaidTier
--- PASS: TestExtractSQLPricing_AllTiersFreeIsSkipped
--- FAIL: TestExtractPricing_FreeFirstTierUsesPaidTier            (memorystore)
--- PASS: TestExtractPricing_AllTiersFreeIsSkipped                (memorystore)
--- FAIL: TestExtractStoragePricing_FreeFirstTierUsesPaidTier
      Max difference between 2.7397260273972603e-05 and 0 ...      (price was 0)
--- PASS: TestExtractStoragePricing_AllTiersFreeIsSkipped

(TestPaidRate tests the new helper, and the AllTiersFree tests are guards that pass before and after the fix.)

Pass-after, run from providers/gcp:

AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./...                  # all ok
GOWORK=off AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -race -short ./...   # all ok
go build ./... && make vet                                                                       # clean
golangci-lint v2.10.1 run --allow-parallel-runners ./...                                         # 0 issues
gocyclo -over 10 internal/skumatch/skumatch.go services/{cloudsql,memorystore,cloudstorage}/client.go  # silent

I set AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE to /dev/null only because the local machine's default AWS profile is invalid. The GCP tests don't depend on it.

Mutation check: if PaidRate goes back to returning TieredRates[0], the three FreeFirstTierUsesPaidTier tests and the free first tier then paid cases in TestPaidRate fail.

Interaction with #424 (`fix/issue-243`)

#424 also edits providers/gcp/services/cloudstorage/client.go, in extractStoragePricingFromSKUs currency handling and the import block. This PR changes only the TieredRates[0] lines in extractStoragePriceFromSKU, adds no import there (skumatch is already imported), and puts its test in a new tiered_rates_test.go instead of #424's pricing_currency_test.go. A trial merge of fork/fix/issue-243 into this branch auto-merged with no conflicts, and go build ./... plus the cloudstorage/skumatch tests passed on the merged tree.

Out of scope

providers/gcp/services/computeengine/pricing.go:259 also reads only TieredRates[0]. The issue doesn't list it, so I left it alone. It could switch to skumatch.PaidRate in a follow-up.
Not checked against the real catalog: whether any Cloud SQL / Memorystore / Cloud Storage SKU actually has a [0, >0] tier shape.

Maintainer: please copy the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/s, type/bug.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Cloud SQL, Cloud Storage, and Memorystore pricing now uses the first paid tier when a free tier comes first, improving on-demand and commitment price accuracy.
- SKUs with only free tiers continue to return zero pricing.

The Cloud SQL, Memorystore and Cloud Storage extractors read only TieredRates[0], so a SKU whose first tier is a free allowance and whose later tier is paid came back as price 0 and was skipped entirely. Add skumatch.PaidRate, which returns the first tier with a non-zero unit price, and use it in all three extractors. SKUs whose tiers are all free are still skipped. Closes LeanerCloud#242 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-16T02:26:33Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 2f76a6a2-f893-486b-90cc-e6f58c87da93

📥 Commits

Reviewing files that changed from the base of the PR and between 8afb655 and 51697ab.

📒 Files selected for processing: 8

providers/gcp/internal/skumatch/paid_rate_test.go
providers/gcp/internal/skumatch/skumatch.go
providers/gcp/services/cloudsql/client.go
providers/gcp/services/cloudsql/tiered_rates_test.go
providers/gcp/services/cloudstorage/client.go
providers/gcp/services/cloudstorage/tiered_rates_test.go
providers/gcp/services/memorystore/client.go
providers/gcp/services/memorystore/tiered_rates_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

GCP pricing extraction now selects the first nonzero tier rate for Cloud SQL, Cloud Storage, and Memorystore. A shared helper handles tier selection. Tests cover SKUs with a free first tier and SKUs with no paid tier.

Changes: GCP tiered pricing

Layer / File(s)	Summary
Select the first paid tier `providers/gcp/internal/skumatch/skumatch.go`, `providers/gcp/internal/skumatch/paid_rate_test.go`	`PaidRate` scans tiers in order and returns the first rate with a non-nil, nonzero unit price. Tests cover nil expressions, empty and free tiers, and paid tiers.
Use paid tiers in pricing extractors `providers/gcp/services/cloudsql/client.go`, `providers/gcp/services/cloudsql/tiered_rates_test.go`, `providers/gcp/services/cloudstorage/client.go`, `providers/gcp/services/cloudstorage/tiered_rates_test.go`, `providers/gcp/services/memorystore/client.go`, `providers/gcp/services/memorystore/tiered_rates_test.go`	The three extractors use `PaidRate`. Tests cover pricing from a paid tier after a free tier, all-free tiers, and commitment pricing where tested.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: cristim

Merge Risk: ⚪ Minimal · up to 51697

No actionable pricing defect remains identified; the change is mergeable after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 8 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: pricing tiered GCP SKUs from the first paid tier.
Linked Issues check		Issue `#242` requires handling a free first tier followed by a paid tier in Cloud SQL, Memorystore, and Cloud Storage, with a test in each extractor. The PR adds `skumatch.PaidRate`, which skips nil and…
Out of Scope Changes check		The diff is limited to the shared tier-rate helper, the three extractors named in issue `#242`, and focused unit tests for those changes. The Compute Engine `TieredRates[0]` use is not changed, which ma…

Full details: Linked Issues check

Explanation

Issue #242 requires handling a free first tier followed by a paid tier in Cloud SQL, Memorystore, and Cloud Storage, with a test in each extractor. The PR adds skumatch.PaidRate, which skips nil and zero-priced tiers and returns the first non-zero tier. All three extractors use it. The new tiered_rates_test.go files cover the free-first-tier case and the all-free case for each service. The implementation matches the issue's accepted first-paid-tier option.

Full details: Out of Scope Changes check

Explanation

The diff is limited to the shared tier-rate helper, the three extractors named in issue #242, and focused unit tests for those changes. The Compute Engine TieredRates[0] use is not changed, which matches the stated issue scope. No unrelated implementation or test changes are shown.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

cristim · 2026-10-16T02:41:47Z

Independent adversarial review: CLEAN at 48dd6971 (claude-go-pr-merge-2, non-author reviewer). No blocking findings.

PaidRate (skumatch.go:101-117) treats a tier as paid when Units or Nanos is non-zero (nanos-only prices count); all-free, nil expression, nil rate and nil UnitPrice return nil, and all three callers keep the previous "no price" handling. Price math Units + Nanos/1e9 and the tier's currency code are correct for Money (e.g. cloudsql/client.go:467-468).
Fail-before: with origin/main's client.go for cloudsql, cloudstorage and memorystore, TestExtractSQLPricing_FreeFirstTierUsesPaidTier, TestExtractStoragePricing_FreeFirstTierUsesPaidTier and TestExtractPricing_FreeFirstTierUsesPaidTier fail; they pass at this head.
Mutations: dropping the Nanos check fails TestPaidRate; iterating TieredRates[:0] fails TestPaidRate and cloudsql tests.
Non-blocking: (1) PaidRate assumes tiers arrive sorted by StartUsageAmount; the API type (cloudbilling-gen.go:1232) does not promise order. Sorting before the scan would be cheap hardening; no known SKU violates it. (2) computeengine/pricing.go:253-265 still reads TieredRates[0] but fails loud on a zero price, and fix(gcp): pricing extractors read only TieredRates[0], so a SKU whose first tier is free is skipped #242 scopes only the three extractors changed here.
Local verification (macOS, unit/mock, no GCP calls): (cd providers/gcp && go test -race -short ./internal/skumatch/... ./services/...) pass; make build, make vet pass.

Still needed before merge: owner approval of the fork CI runs and green CI on this SHA, and the author marking the PR ready. Labels mirrored from #242. CR skipped: quota/draft; the merge comment will carry CR waived: quota, adversarial review + local verification + green CI once CI is green.

cristim · 2026-10-17T18:49:21Z

Merge gate record at 51697abb737d6fc692e77f1ddbf7613edb4fc10c (merge driver)

Independent review CLEAN at 48dd6971 (verdict above). Each first-parent commit after it is a merge of main, and each merge's tree equals git merge-tree --write-tree of its parents. The PR's own change is therefore unchanged and the verdict stands. The latest merge is GitHub's Update branch, which the author consented to on fix(azure): attribute shared-scope vm reservations to their billing subscription #423.
Local verification on macOS at this head (fixture/mock evidence, no live cloud calls): cd providers/gcp && go test -race -short ./... && go vet ./... passes.
CI green on this head: CI - Build & Test 39058631558, pre-commit 39058631508. mergeState CLEAN, no unresolved threads, labels mirrored from fix(gcp): pricing extractors read only TieredRates[0], so a SKU whose first tier is free is skipped #242. CR waived (quota): adversarial review + local verification + green CI.

Squash-merging with --match-head-commit 51697abb.

cristim added priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/s Hours type/bug Defect triaged Item has been triaged labels Oct 16, 2026

IAmJSD marked this pull request as ready for review October 16, 2026 10:27

IAmJSD mentioned this pull request Oct 16, 2026

fix(gcp): fail closed on region-less storage SKUs and ambiguous SKU matches #471

Open

cristim mentioned this pull request Oct 16, 2026

fix(gcp/memorystore): list Redis SKUs from the Cloud Memorystore for Redis billing service #483

Merged

IAmJSD mentioned this pull request Oct 17, 2026

fix(gcp): stop emitting Cloud SQL and Memorystore CUDs from PerformanceRecommenders #493

Merged

Merge branch 'main' into fix/issue-242

51697ab

cristim merged commit 364131a into LeanerCloud:main Oct 17, 2026
15 checks passed

cristim mentioned this pull request Oct 17, 2026

refactor(aws): share the purchase idempotency guard, starting with ElastiCache #507

Merged

Navigation Menu

Repository navigation

fix(gcp): price tiered SKUs from the first paid tier - #433

fix(gcp): price tiered SKUs from the first paid tier#433
cristim merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-242

IAmJSD commented Oct 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

cristim commented Oct 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

IAmJSD commented Oct 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Fix

Verification

Interaction with #424 (fix/issue-243)

Out of scope

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Oct 16, 2026

Uh oh!

cristim commented Oct 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IAmJSD commented Oct 16, 2026 •

edited by coderabbitai Bot

Loading

Interaction with #424 (`fix/issue-243`)

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading